Preserve Gmail labels in the mbox as sub-labels of the main mbox label. by dotysan · Pull Request #32 · google/import-mailbox-to-gmail

dotysan · 2017-10-31T22:42:24Z

Also ignore messages in Trash/Spam.
Don't bother creating Unread label.

- Also ignore messages in Trash/Spam. - Don't bother creating Unread label.

googlebot · 2017-10-31T22:42:27Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

dotysan · 2017-10-31T22:48:25Z

I signed the CLA.

googlebot · 2017-10-31T22:48:28Z

CLAs look good, thanks!

eesheesh

Hi,

Thanks for this! I've been wanting to add Google Takeout support for a while but never had the time.

I have quite a few changes I'd like to see before approving it. I understand that you may not have time to do it, so if you don't - that's fine, please let me know.

eesheesh · 2017-11-06T17:10:04Z

 import logging
 import logging.handlers
 import mailbox
+from csv import reader


Please order imports alphabetically

OK, willdo.

eesheesh · 2017-11-06T17:12:44Z

+    dest='with_labels',
+    required=False,
+    action='store_false',
+    help=


This is meant for MBOXes created by Google Takeout, right? In that case, I think it would be more helpful to say "Preserve Gmail labels from Takeout mbox files" and naming it "--takeout-labels".

Also, I think there's a lot of value in importing read/unread status and spam/trash for some people, so I'd suggest creating separate boolean flags for "--takeout-read-state" and "--takeout-spam-trash".

Agreed. I was just lazy. Will implement these suggestions now.

eesheesh · 2017-11-06T17:13:26Z

        logging.info("Processing message %d in label '%s'", index, labelname)
+
+        if args.with_labels and 'X-Gmail-Labels' in message:
+          gmail_labels= next(reader([message['X-Gmail-Labels']]))


Won't this get only the first label?

Also: Have you tested what happens if there's a label with a comma in its name in Gmail before the Takeout export?

Yes. That's why I used from csv import reader. However in testing, I found that I forgot to handle a long list of labels that wraps the header. Will fix that now too.

eesheesh · 2017-11-06T17:14:07Z

+
+        if args.with_labels and 'X-Gmail-Labels' in message:
+          gmail_labels= next(reader([message['X-Gmail-Labels']]))
+          #logging.info('Found Gmail Labels: %s', gmail_labels)


Why not log it? I think it's valuable.

Me too! Just a bit chatty. I'll remove the comment.

eesheesh · 2017-11-06T17:14:55Z

+            logging.info("Skipped Trash message %d in label '%s'", index, labelname)
+            continue
+          if 'Unread' in gmail_labels:
+            gmail_labels.remove('Unread')


See my note above in the flags - I think this should be controlled by the user (and specifically the unread label should be imported by default).

OK. However, if I import the 'Unread' label as a sub-label, it doesn't work as expected. So I'll add the code/arg you suggested and make sure it is imported without the sub-label adjustment.

eesheesh · 2017-11-06T17:19:38Z

+          if 'Unread' in gmail_labels:
+            gmail_labels.remove('Unread')
+
+          label_ids= [label_id]


Please make spacing consistent with the rest of the code (here and below).

eesheesh · 2017-11-06T17:21:02Z

+            gmail_labels.remove('Unread')
+
+          label_ids= [label_id]
+          for sublabel in gmail_labels:


I don't think this will work with how you use next() above.

>>> from csv import reader >>> gmail_labels= next(reader("a,b,c")) >>> gmail_labels ['a'] >>> next(gmail_labels) Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: list object is not an iterator >>> for sublabel in gmail_labels: ... print sublabel ... a

It works for me. FYI, I actually did this:

>>> from csv import reader >>> gmail_labels= next(reader(["a,b,c"])) >>> gmail_labels ['a', 'b', 'c']

eesheesh · 2017-11-06T17:23:05Z

+            label_ids.append(get_label_id_from_name(service, username, labels, sublabel))
+          metadata_object= {'labelIds': label_ids}
+
+        # TODO: test/handle when there is no labels header?


Isn't this already handled by "if args.with_labels and 'X-Gmail-Labels' in message" above? What else needs to be tested?

Oops. It was a note-to-self written before I did the deed. Should have removed it before the PR.

eesheesh · 2017-11-06T17:26:59Z

          logging.exception(
              'Failed to replace text/quoted-printable with text/plain '
              'in Content-Type header')
+


Why are there new empty lines here and elsewhere?

Was just for my own personal readability/style. I'll clean it up and make it consistent with yours.

eesheesh · 2017-11-06T17:27:42Z

          continue
        logging.info("Processing message %d in label '%s'", index, labelname)
+
+        if args.with_labels and 'X-Gmail-Labels' in message:


This is quite long, maybe extract it into a separate function?

Can do. I was just following existing convention in the code. I.e.
if args.fix_msgid and 'Message-ID' in message:

dotysan · 2017-11-06T23:45:16Z

Here's what I've implemented:

  --takeout-labels {True,False,sublabels}
                        Preserve Gmail labels from Takeout mbox files. Or
                        optionally, transform them into sublabels. (default:
                        keep them)
  --takeout-no-unread   Don't preserve read/unread state from Takeout mbox
                        files (default: keep it)
  --takeout-spam-trash  Import messages from Spam/Trash labels in Takeout mbox
                        files (default: skip them)

...still hafta test that last one!

eesheesh

Some minor changes requested. Have you tested all of the new options yet? (as well as with a "normal" mbox in a directory structure to ensure no regressions)

Thanks again for your help!

eesheesh · 2017-11-10T21:47:54Z

+    dest='takeout_labels',
+    choices=[True, False, 'sublabels'],
+    required=False,
+    default=True,


Please remove :)

eesheesh · 2017-11-10T21:51:00Z

+
+  if args.takeout_labels and 'X-Gmail-Labels' in msg:
+    gmail_labels = next(reader([msg['X-Gmail-Labels'].replace('\r\n', '')]))
+    logging.info('Found Gmail Labels: %s', gmail_labels)


Maybe "Found Gmail labels from Takeout"? Otherwise the user might not understand the context (if they don't remember this mbox came from Takeout, etc.), this would help differentiate it from finding a label in the sense of finding a new mbox file.

eesheesh · 2017-11-10T21:52:31Z

+    logging.info('Found Gmail Labels: %s', gmail_labels)
+
+    if args.takeout_spam_trash:
+      # TODO: test if we are importing spam/trash without renaming to sublabels


Are we not testing that?

eesheesh · 2017-11-10T21:52:49Z

+
+    for gmail_label in gmail_labels:
+      if args.takeout_labels == 'sublabels' and gmail_label != 'Unread':
+        gmail_label = "%s/%s" %(mbox_label_name, gmail_label)


Space after % please :)

eesheesh · 2017-11-10T21:55:13Z

          logging.exception('Failed to fix brackets in Message-ID header')
-        metadata_object = {'labelIds': [label_id]}
+        metadata_object = get_metadata(service, username, labels, message, label_id, labelname)
+        if not metadata_object: continue


Please put continue in a new indented line.

When does this happen? Only if this is a spam/trash label we're skipping?

eesheesh · 2017-11-10T21:58:35Z

    raise


+# TODO: instead of passing mbox_label_name as param, can't we look it up from id?


We can, but it would be less efficient (potentially much slower if you have many labels and many messages). I think we can drop this comment unless you feel strongly about it.

eesheesh · 2017-11-29T22:43:52Z

Hi @dotysan, please let me know if you don't have time to make the changes I requested so I can take it up myself if needed. :) Thanks!

dotysan · 2017-11-30T03:04:38Z

Unfortunately, I haven't had a chance to test much. Plus, I got hung up trying to decide what SYSTEM labels to honor when importing into a sublabel...

For example, in the current PR, when importing --takeout-labels sublabels without --takeout-no-unread it will honor the UNREAD system label instead of creating a sublabel. This preserves the bold formatting of that status. But it will still create 'Starred' and 'Important' sublabels.

In my un-pushed tree, I've also honored the STARRED and IMPORTANT system labels and subsequent status formatting. Still fiddling with it a bit more. Feedback?

eesheesh · 2017-12-07T11:47:40Z

Hi, sorry for the delay in response, I was away.

I think treating STARRED and IMPORTANT as system labels (i.e. not prefixing them) is good, and IMHO there's no need in adding a flag on whether to preserve them like read/unread.

Thanks!

dotysan · 2018-06-22T20:11:57Z

So I'm finally back on this and testing/refining. And just stumbled across an annoying (redundant?) label that is inconsistently applied. https://productforums.google.com/forum/#!topic/gmail/eKveqdKxZDM

Think I'll add the code to totally ignore "Archived" label since it's already implied with the absence of the "Inbox" label. Sound good?

eesheesh · 2018-06-22T20:53:45Z

Think I'll add the code to totally ignore "Archived" label since it's already implied with the absence of the "Inbox" label. Sound good?

Yes, if there's no other label it will go to All Mail which is the same as archived. Thanks!

Preserve Gmail labels in the mbox as sub-labels of the main mbox label.

3a7885a

- Also ignore messages in Trash/Spam. - Don't bother creating Unread label.

googlebot added the cla: no label Oct 31, 2017

googlebot added cla: yes and removed cla: no labels Oct 31, 2017

Add option to disable import of Gmail labels.

cfa0c78

eesheesh requested changes Nov 6, 2017

View reviewed changes

dotysan added 2 commits November 6, 2017 15:40

Attempting to address feedback from: google#32 (review)

c356b82

tyop

5560c21

dotysan added 2 commits November 6, 2017 15:52

Eradicate vestiges of accidental whitepsace invasion.

f2e186c

Oh drat. I'm blind.

4e4d39e

eesheesh requested changes Nov 10, 2017

View reviewed changes

katrkatcloud approved these changes Feb 10, 2023

View reviewed changes

		raise


		# TODO: instead of passing mbox_label_name as param, can't we look it up from id?

Conversation

dotysan commented Oct 31, 2017

Uh oh!

googlebot commented Oct 31, 2017

Uh oh!

dotysan commented Oct 31, 2017

Uh oh!

googlebot commented Oct 31, 2017

Uh oh!

eesheesh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dotysan commented Nov 6, 2017

Uh oh!

eesheesh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eesheesh commented Nov 29, 2017

Uh oh!

dotysan commented Nov 30, 2017

Uh oh!

eesheesh commented Dec 7, 2017

Uh oh!

dotysan commented Jun 22, 2018

Uh oh!

eesheesh commented Jun 22, 2018

Uh oh!

Reviewers